Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: custom vesting module to check signer in allow list #387

Closed
wants to merge 5 commits into from

Conversation

trinitys7
Copy link
Contributor

@trinitys7 trinitys7 commented Apr 23, 2024

As mentioned in rollapp-evm . We need to custom vesting module for creating a allow list that can create vest account.
The code is taken from cosmos-sdk with a few changes for allow list.

@trinitys7 trinitys7 requested a review from a team as a code owner April 23, 2024 05:56
Copy link
Contributor

@omritoptix omritoptix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we write genesis tests and ante handler test please?
also if we can add a readme.md about this module with a simple explanation of what it does.

for _, disabledTypeURL := range pvd.disabledMsgTypeURLs {
if typeURL == disabledTypeURL {
// Check if vesting tx signer is 1
if len(msg.GetSigners()) != 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we blocking multisigs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see, all vesting msgs on cosmos-sdk or evmos only contains 1 signer. WDYT

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok we can start with that

testutil/keepers/keepers.go Dismissed Show dismissed Hide dismissed
@mtsitrin
Copy link
Collaborator

@omritoptix isn't it bit redundent to have a dedicated module to keep the permission address?
the module itself doesn't have any logic, just provides the ante decorator, which can leave outside a module.

Can't we reuse the whitelisted address from other module? it would be the 3rd place we manage whitelist param

@mtsitrin
Copy link
Collaborator

we need to consider how to enforce it through Authz flow

  • either run same verification on Authz flow
  • or block vesting Msg through Authz

)

// PermissionedVestingDecorator prevents invalid msg types from being executed
type PermissionedVestingDecorator struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not wired in app.go ante handler

Copy link
Contributor Author

@trinitys7 trinitys7 Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the dymension-rdk doesn't actually have antehandler. So I added it here then on rollapp-evm's app.go, we just need to call the Decorator from here

@omritoptix
Copy link
Contributor

Can't we reuse the whitelisted address from other module? it would be the 3rd place we manage whitelist param

@mtsitrin yea thought about it initially but imo it's less than ideal for 2 reasons:

  1. both modules which have currently the whitelisted addresses (i.e hubgenesis and denom-metadata) are temporary as we plan to automate this through ibc from the hub but the vesting whitelist is supposedly a long term solution.
  2. it's not necessarily the same actors (i.e you may want to give whitelist vesting permissions to a wider crowd)

@trinitys7
Copy link
Contributor Author

Close and decide to not have vesting module on rdk just for permissioned addresses

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants